Skip to content

Buildings - #2328

Open
exeea wants to merge 20 commits into
MegaMek:mainfrom
exeea:buildings
Open

Buildings#2328
exeea wants to merge 20 commits into
MegaMek:mainfrom
exeea:buildings

Conversation

@exeea

@exeea exeea commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

This PR adds Buildings and Mobile Buildings construction

depends on MegaMek/megamek#8946

@exeea
exeea requested a review from a team as a code owner September 11, 2026 13:39
Copilot AI lite review requested due to automatic review settings September 11, 2026 13:39

private static final Map<SmallCraft, CrewConfiguration> AUTO_FILLED_CREW = new WeakHashMap<>();
// Entity equality uses mutable game IDs; separate unassigned craft all have ID -1.
private static final Map<SmallCraft, CrewConfiguration> AUTO_FILLED_CREW =

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a mini fix unrelated

private Element element(Element parent, String name, String... attributes) {
Element element = getSVGDocument().createElementNS(svgNS, name);
for (int i = 0; i < attributes.length; i += 2) {
element.setAttribute(attributes[i], attributes[i + 1]);
Comment thread megameklab/src/megameklab/printing/PrintBuilding.java Fixed
}

static int floor(AbstractBuildingEntity entity, String text) {
return Math.toIntExact(("Ground".equals(text) ? 0 : Long.parseLong(text)) - BuildingConstruction.baseLevel(entity));
assertEquals(List.of("7", "6", "5", "4", "3", "2"), elements(sheet, "g", "building-map-layer").stream()
.map(e -> e.getAttribute("data-building-floor")).toList());
assertEquals(24, elements(sheet, "g", "building-inventory-entry").stream()
.filter(row -> Integer.parseInt(row.getAttribute("data-location")) >= 0).count());
Copilot stopped reviewing on behalf of exeea due to an error September 11, 2026 13:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Adds full Building/Mobile Structure support across the editor, printing pipeline, and verification logic, plus extensive new test coverage for building geometry/services and record sheet output.

Changes:

  • Introduce Building editor UI (structure/placement/equipment/transport/systems) and integrate it into menus and UI loader.
  • Add building record sheet printing (map layers, inventory pagination, doors/elevators/features) and include buildings in print queue.
  • Fix/adjust related mechanics and printing behaviors (capital bay AV rounding, infantry crew-operated range mods, bay ammo selection, crew autofill identity), with new unit/UI/printing tests and fixture BLKs.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
megameklab/unittests/megameklab/util/UnitUtilTest.java Adds targeted regression tests for building armor capacity and crew identity behavior.
megameklab/unittests/megameklab/util/BuildingUtilTest.java Adds unit tests for building geometry edits, placement round-trips, and labels.
megameklab/unittests/megameklab/ui/building/BuildingMainUITest.java Adds Swing-level integration tests covering building editor workflows and undo/redo.
megameklab/unittests/megameklab/printing/WeaponBayDamageTest.java Verifies capital bay damage rounding after fractional contributions.
megameklab/unittests/megameklab/printing/PrintInfantryRangeTest.java Tests crew-operated/underwater range modifier printing behavior.
megameklab/unittests/megameklab/printing/PrintBuildingTest.java Adds record-sheet rendering/layout tests for buildings (layers, features, paging).
megameklab/unittests/megameklab/printing/InventoryWriterBayAmmoTest.java Tests bay ammo association now sourced from the bay itself.
megameklab/testresources/Longinus metadata lifecycle.blk Adds metadata-lifecycle fixture for BattleArmor.
megameklab/testresources/Dragonstar metadata lifecycle.blk Adds metadata-lifecycle fixture for SmallCraft.
megameklab/src/megameklab/util/UnitUtil.java Adds building handling for max armor points, verifier selection, and editor-type resolution.
megameklab/src/megameklab/util/UnitPrintManager.java Adds PrintBuilding sheet creation for building entities.
megameklab/src/megameklab/util/CConfig.java Adds window-position persistence key for BuildingMainUI.
megameklab/src/megameklab/util/BuildingUtil.java New shared building construction/coordinate/utilities used by UI and printing.
megameklab/src/megameklab/util/BuildingMap.java New shared “feature” model for map rendering and printed structure maps.
megameklab/src/megameklab/util/AeroUtil.java Fixes auto-filled crew tracking via identity-based weak keys.
megameklab/src/megameklab/ui/util/EquipmentDatabaseCategory.java Expands category visibility rules to support building equipment use-cases.
megameklab/src/megameklab/ui/generalUnit/TransportTab.java Keeps building bay placement metadata consistent when bays are rebuilt/resized.
megameklab/src/megameklab/ui/dialog/UiLoader.java Routes building entity types to BuildingMainUI construction editor.
megameklab/src/megameklab/ui/building/BuildingTransportTab.java New building-specific transport + quarters UI with placement dialogs.
megameklab/src/megameklab/ui/building/BuildingSystemsTab.java New construction options/service access UI plus always-on validation report.
megameklab/src/megameklab/ui/building/BuildingStructureTab.java New footprint editor (top/pancake), geometry transforms, fuel/levels controls.
megameklab/src/megameklab/ui/building/BuildingPlacementDialogs.java New placement editors for equipment distribution, bays, doors, and elevators.
megameklab/src/megameklab/ui/building/BuildingMainUI.java New main Building editor container and refresh/location-selection logic.
megameklab/src/megameklab/ui/building/BuildingEquipmentTab.java New building equipment placement UI + filtered equipment database view.
megameklab/src/megameklab/ui/StartupGUI.java Adds “New Building” entry to startup menu.
megameklab/src/megameklab/ui/MenuBar.java Adds “Building” to File → New menu.
megameklab/src/megameklab/ui/MegaMekLabTabbedUI.java Adds new-tab creation entries for Building and Mobile Structure.
megameklab/src/megameklab/printing/WeaponBayInventoryEntry.java Fixes capital bay AV formatting to round correctly.
megameklab/src/megameklab/printing/SVGMassPrinter.java Adjusts export toggles and refactors UnitData metadata read timing.
megameklab/src/megameklab/printing/PrintInfantry.java Adjusts range modifier logic to account for crew-operated weapons.
megameklab/src/megameklab/printing/PrintBuilding.java New building record sheet implementation (maps, inventory, services, paging).
megameklab/src/megameklab/printing/InventoryWriter.java Changes bay ammo association and exposes constants/helpers for planning pages.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread megameklab/src/megameklab/util/CConfig.java Outdated
Comment thread megameklab/src/megameklab/printing/SVGMassPrinter.java
Comment thread megameklab/src/megameklab/ui/building/BuildingMainUI.java Outdated
Comment on lines +100 to +107
public static int exteriorFacing(AbstractBuildingEntity entity, CubeCoords hex) {
for (int side = 0; side < 6; side++) {
if (!entity.getInternalBuilding().getOriginalCoordsList().contains(hex.toOffset().translated(side).toCube())) {
return side;
}
}
return 0;
}
Comment on lines +744 to +750
Path output = Path.of("build", "building-review", name + ".png");
Files.createDirectories(output.getParent());
ImageIO.write(image, "png", output.toFile());
} catch (Exception exception) {
fail(exception);
} finally {
editor.removeNotify();
Comment thread megameklab/src/megameklab/util/BuildingMap.java

@IllianiBird IllianiBird left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly approved, but there are some areas of concern. The chief among them is the mass use of hardcoded player facing Strings. These should be stored in a RESOURCE_BUNDLE and accessed with whatever the mml equivalent of getTextAt is.

Comment on lines +481 to +485
for (WeaponMounted bay : weapons) {
WeaponBayText wbt = new WeaponBayText(bay.getLocation(), bay.isRearMounted());
for (WeaponMounted weaponMounted : bay.getBayWeapons()) {
if (!wbt.addBayWeapon(weaponMounted)) {continue;}
for (AmmoMounted ammo : ammoMountedList) {
if (ammo.getLocation() == weaponMounted.getLocation()
&& weaponMounted.getType().getAmmoType() == ammo.getType().getAmmoType()) {
for (AmmoMounted ammo : bay.getBayAmmo()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way for us to avoid nesting a loop within a loop within a loop?

@Override
protected String getRecordSheetTitle() {
if (building instanceof MobileStructure) {
return "Mobile Structure Record Sheet";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded player facing strings should be avoided

BuildingSystemsTab(BuildingMainUI editor) {
this.editor = editor;
setLayout(new BorderLayout(8, 8));
setBorder(BorderFactory.createEmptyBorder(8, 8, 8, 8));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be using scaleForGUI. We should avoid hardcoding gui sizes as it won't react to user changes to the GUI Scale client option

Comment thread megameklab/src/megameklab/printing/PrintBuilding.java Fixed
@HammerGS

Copy link
Copy Markdown
Member

Review: MegaMekLab PR #2328 "Buildings" (exeea/buildings -> main)

Reviewed at head 56999c7 (merge-base = current main 44081e5). 32 files, +5,909 / -50. Depends on MegaMek
PR #8946; compiled and tested locally against that branch. Checked against the project coding style guide and PR checklist.
Each item: what was checked, what was found, what to do.

Verdict: NOT MERGEABLE as-is. Blocked on #8946, on red CI of its own, and on two hard gates (no i18n, Swing
tests). The model/util layer (BuildingUtil, BuildingMap, BuildingUtilTest) is the right shape; the UI and
printing layers need the standard MML treatment before they can be reviewed for behaviour.


A. Blockers

# Check Finding Recommendation
B1 Dependency Requires MegaMek #8946 (AbstractBuildingEntity, BuildingDesign, TestBuilding, ETYPE_MOBILE_STRUCTURE). CI here fails on the MegaMek compileTestJava and Javadoc errors first. Land #8946 (split) first; rebase.
B2 CI green checkstyle: Spotless violations in 12 files (import order in PrintBuilding, BuildingEquipmentTab, ...). pr_ci: 22 test failures (see B3). Reproduced locally: Spotless fails; main and test sources compile against the #8946 branch. spotlessApply; fix or remove the failing tests.
B3 Own tests pass PrintBuildingTest.printQueueAccountsForAllBuildingPages: NPE at PrintBuilding.inventoryPages:511 through getPageCount -> UnitPrintManager.createSheets (reproduced locally with a display, so not a headless artefact; template.getElementById("inventory") or loadTemplate returns null on that path). PrintBuildingTest.ammoQuantitiesQuartersAndPdf...: FOP FontEventListener NPE inside exportPDF (environment/font config; the same call works for other sheets only when fonts are configured). BuildingMainUITest: 20 cases HeadlessException in CI, 1 case ClassCastException at :596 locally. Fix the inventoryPages template lookup and add a plain unit test on the page-planning maths. Drop the PDF export from unit tests. Remove BuildingMainUITest (B5).
B4 No hardcoded user-facing strings FAIL, hard gate. Zero resource lookups in the six new UI classes; existing tabs use ResourceBundle.getBundle("megameklab.resources.Tabs") and resourceMap.getString. Hardcoded English literals: BuildingStructureTab 65, BuildingSystemsTab 64, BuildingPlacementDialogs 25, BuildingMainUI 18, BuildingEquipmentTab 16, BuildingTransportTab 9. Examples: new JCheckBox("Show all hexes and floors"), new JButton("Distribute equipment mass…"), `"Installed: %.2f / %.2f tons Power: %s
B5 No Swing component tests FAIL. BuildingMainUITest (777 lines) constructs BuildingMainUI on the EDT, finds components by name, drives tabs/tables/undo, renders PNGs to the working directory. Maintainer position (IllianiBird, MekHQ #9940): "we shouldn't be testing gui components, that road leads to madness." It also fails in CI. Delete it. Keep BuildingUtilTest and UnitUtilTest (model-level, correct shape). If editor logic needs coverage, extract it into a plain model class and test that.
B6 Scope Unrelated changes ride along: AeroUtil identity-keyed map (author's own inline comment: "this is a mini fix unrelated"); PrintInfantry crew-served weapon range modifier; WeaponBayInventoryEntry AV rounding; InventoryWriter bay-ammo association switched from location matching to bay.getBayAmmo() + AmmoType.isAmmoValid (changes every large-craft record sheet); SVGMassPrinter defaults flipped (SKIP_UNIT_FILES = false, EXPORT_CALCULATIONS_AS_TEXT = true). Each has its own test file. Split into separate PRs with their tests. Revert the two SVGMassPrinter toggles here: it is a main() tool and now rewrites unit files by default.

B. Style and hygiene

# Check Finding Recommendation
S1 UIUtil.scaleForGUI on pixel values 0 uses. Raw values: loadout.setPreferredSize(new Dimension(850, 250)), scroll.setPreferredSize(new Dimension(650, 320)), Insets(0, 0, 8, 0), Insets(2, 0, 2, 8) etc. (9 sites). Wrap. Regular review callout (IllianiBird, MM #8657).
S2 No Unicode in code ×, , in PrintBuilding (lines ~413, 436, 447, 471) and in button labels. 22 added lines. ASCII (x, -, ...). Moot for labels once they move to properties, but the record-sheet text must be ASCII-safe too.
S3 No fully-qualified names mid-code 13 sites: BuildingUtil 7, BuildingMainUI 2, BuildingPlacementDialogs 2, BuildingSystemsTab 2 (e.g. megamek.common.equipment.enums.StructureEngine.COMBUSTION_LIQUID, megamek.common.units.BuildingEntity). Import.
S4 var only when obvious; no single-letter lambdas 352 var declarations; e -> 46, m -> 9, c -> 7. Explicit types; full names.
S5 Javadoc / license New files carry the header. Public methods in BuildingUtil/BuildingMap mostly documented; the tab classes largely not. Document public entry points.
S6 Diagnostic logging None in the 9 new source files. For pure Swing plumbing that is tolerable; not for PrintBuilding paging (inventoryPages, continuation pages) or UnitUtil verifier/editor-type resolution, which is where "the sheet is blank / wrong editor opened" reports will land. Debug lines on the page planner and on UnitUtil.getEditorTypeForEntity fallthrough.
S7 @Nullable BuildingUtil and BuildingMap return null/Optional mixed; no annotations. Annotate.

C. Automated review triage

Source Location Verdict Reason
CodeQL 3389 PrintBuilding.java:623 varargs pair index may be out of bounds VALID (low) element(parent, name, String... attributes) reads attributes[i + 1]; all current callers pass pairs. Assert even length or take a Map.
CodeQL 3388 PrintBuilding.java:175 row >= rows always false VALID (trivial) The enclosing loop already bounds row < rows. Delete the clause.
CodeQL 3386 BuildingPlacementDialogs.java:301 Long.parseLong(text) VALID (medium) floor(entity, text) parses a table cell the user typed; any non-number crashes the dialog. Catch, fall back, and mark the cell.
CodeQL 3387 PrintBuildingTest.java:423 same Moot Test code; goes with the test rewrite.
Copilot CConfig.java:715 HHWMainUI returns the WarShip window key OUT OF SCOPE Pre-existing on main; the PR only adds the BuildingMainUI branch below it. Real bug, own issue.
Copilot BuildingMainUI.java:217 Math.clamp DECLINE Build targets Java 21; Math.clamp is a 21 API.
Copilot SVGMassPrinter.java:151 flipped defaults VALID See B6.
Copilot BuildingUtil.java:107 six contains() on a list per call PLAUSIBLE (low) Called per hex during painting; a Set<CubeCoords> built once per refresh is cheap.
Copilot BuildingMap.java:89 list + stream per fill call PLAUSIBLE (low) Static constant list.
Copilot BuildingMainUITest.java:750 PNGs written to the working dir VALID Moot once the test is removed.

D. Printing pipeline notes

  • PrintBuilding.getSVGFileName returns building_default.svg; the template landed in mm-data Fuel mass for advanced aerospace craft appears to be rounded down to the nearest whole ton #562 (merged 2026-09-11 13:06, before this PR) with id="inventory" present. The NPE at :511 is therefore in the getSVGDocument() == null ? loadTemplate(...) : getSVGDocument() path, not a missing asset. Reproduce with UnitPrintManager.createSheets(List.of(building, building2), true, options) as the test does.
  • InventoryWriter constants MIN_FONT_SIZE / MIN_LINE_HEIGHT_TO_FONT_SIZE widened from private to package-private and scaleText made static for the building planner: fine, but it couples building paging to inventory internals. A small InventoryLayout helper would keep InventoryWriter closed.

E. PR body and process

  • Body: "This PR adds Buildings and Mobile Buildings construction, depends on #8946". Template unused; no Testing / What is not proven; no issue link; no AI Assisted Development label.
  • Say what was done live: opened a building in the editor, saved a .blk, reloaded it in MegaMek, printed a sheet.

F. What is done well (keep)

  • BuildingUtil / BuildingMap as plain model classes with BuildingUtilTest and UnitUtilTest: the right split, and what the rest of the UI tests should have been.
  • UiLoader, MenuBar, StartupGUI, MegaMekLabTabbedUI, UnitPrintManager integration points are minimal and follow the existing pattern.
  • TransportTab keeps BuildingDesign bay placement in sync when bays are rebuilt.

@HammerGS

Copy link
Copy Markdown
Member

Follow-up: duplicated logic and hex naming

Items that touch this repo, following the duplication pass on MegaMek #8946.

  • BuildingUtil.rotate is the third copy of the same 60-degree cube rotation (AbstractBuildingEntity.rotateCoordByFacing on MegaMek main, MobileStructureLinkage.rotate in #8946). Recommendation over there is one public CubeCoords.rotate(int turns); this lambda should call it.
  • Hex naming will diverge between the two apps. This PR adds BuildingUtil.locationLabel (sheet-grid label plus level) and absoluteHexLabel (q,r). The in-progress MegaMek branch Implement-Building-Editor-Hex-Layout adds BuildingHexNames.hexName driven by AbstractBuildingEntity.getHexIndex(location) for the in-game damage editor. The same hex would carry one name on the sheet and another in the game. The naming belongs in one helper in megamek.common that both apps call; happy to coordinate on which scheme wins.
  • Checked and fine: BuildingUtil and the tabs call BuildingConstruction and TestBuilding for every rule (usesHexsides, limits, occupiesMapLevel, capacityInHex, generatorTons, and so on) rather than re-deriving them. That is the right split and should stay that way.

@HammerGS

Copy link
Copy Markdown
Member

Recommendation: resolving the duplication (this repo's share)

Full plan is on MegaMek #8946. The parts that land here:

  • Rotation: once CubeCoords.rotate(int turns) exists in MegaMek, the lambda in BuildingUtil.rotate becomes hex -> hex.rotate(1). Rotation preserves the coordinate list order, which item 5 below relies on.
  • Hex naming: the hex identity becomes its 1-based number in the unit file's declaration order (AbstractBuildingEntity.getHexIndex / getHexNumber, coming from the MegaMek side), with a shared BuildingHexLabels helper in megamek.common.units providing hexName ("Hex 3") and locationName ("Hex 3, Level 2").
  • Sheet view: keep SheetGrid for drawing the map, but print the hex number inside every drawn hex and use locationName in the inventory location column. The "0104" grid label stays a drawing coordinate; absoluteHexLabel (q,r) should not appear in anything a person reads.
  • Result: "Hex 3, Level 2" on the printed sheet is the same hex the in-game damage editor calls "Hex 3, Level 2".
  • Order: this lands after the MegaMek shared-model slice that carries the rotation method and the naming helper.

}
// we matched all
return true;
Map<WeaponType, Map<EquipmentType, Integer>> augmentations = new HashMap<>();
"0502", "0503", "0504", "0505", "0506", "0602", "0603", "0604", "0703", "0705", "0805", "0906", "1006", "1107",
"1207", "1308", "1408", "1509", "1609", "1710", "1810", "1911");
var branched = labels.stream().map(label -> {
int q = Integer.parseInt(label.substring(0, 2)) - 1;
"1207", "1308", "1408", "1509", "1609", "1710", "1810", "1911");
var branched = labels.stream().map(label -> {
int q = Integer.parseInt(label.substring(0, 2)) - 1;
int r = Integer.parseInt(label.substring(2)) - 1 - q / 2;
"0401", "0402", "0403", "0404", "0502", "0503", "0504", "0604", "0605", "0606", "0704", "0705",
"0803", "0805", "0906", "1006", "1107", "1207", "1308", "1408", "1509", "1609");
var hexes = labels.stream().map(label -> {
int q = Integer.parseInt(label.substring(0, 2)) - 1;
"0803", "0805", "0906", "1006", "1107", "1207", "1308", "1408", "1509", "1609");
var hexes = labels.stream().map(label -> {
int q = Integer.parseInt(label.substring(0, 2)) - 1;
int r = Integer.parseInt(label.substring(2)) - 1 - q / 2;
"0502", "0503", "0504", "0601", "0602", "0603", "0604", "0703", "0704", "0804", "0905", "1005",
"1106", "1206", "1307", "1407", "1508", "1608", "1709", "1809", "1910", "2010");
var hexes = labels.stream().map(label -> {
int q = Integer.parseInt(label.substring(0, 2)) - 1;
"1106", "1206", "1307", "1407", "1508", "1608", "1709", "1809", "1910", "2010");
var hexes = labels.stream().map(label -> {
int q = Integer.parseInt(label.substring(0, 2)) - 1;
int r = Integer.parseInt(label.substring(2)) - 1 - q / 2;
assertEquals(2, largeDoor.getElementsByTagName("polygon").getLength());
assertEquals(1, largeDoor.getElementsByTagName("line").getLength());
assertTrue(elements(sheet, "polyline", "linked-door-opening").stream()
.allMatch(line -> Double.parseDouble(line.getAttribute("stroke-width")) > 2));
"0401", "0402", "0403", "0404", "0502", "0503", "0504", "0604", "0705", "0805", "0906");
var hexes = labels.stream().map(label -> {
int q = Integer.parseInt(label.substring(0, 2)) - 1;
int r = Integer.parseInt(label.substring(2)) - 1 - q / 2;
"0502", "0503", "0504", "0601", "0602", "0603", "0604", "0703", "0704", "0804", "0905", "1005",
"1106", "1206", "1307", "1407", "1508", "1608", "1709", "1809", "1910", "2010");
var hexes = labels.stream().map(label -> {
int q = Integer.parseInt(label.substring(0, 2)) - 1;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants